Skip to content

Plan 85: extract shared type-conversion helpers (Phase 1) - #150

Merged
jeduden merged 2 commits into
mainfrom
claude/plan-85-shared-helpers
Apr 21, 2026
Merged

Plan 85: extract shared type-conversion helpers (Phase 1)#150
jeduden merged 2 commits into
mainfrom
claude/plan-85-shared-helpers

Conversation

@jeduden

@jeduden jeduden commented Apr 20, 2026

Copy link
Copy Markdown
Owner

Summary

Consolidate duplicated type conversion logic across 10 rule packages into a new shared internal/rules/settings package. This eliminates code duplication and provides a single, well-tested implementation for coercing YAML-decoded values to Go types.

Key Changes

  • New package: Created internal/rules/settings/settings.go with three exported helper functions:

    • ToInt(v any) (int, bool) - Coerces int, int64, or float64 to int (truncating floats toward zero)
    • ToFloat(v any) (float64, bool) - Coerces int, int64, or float64 to float64
    • ToStringSlice(v any) ([]string, bool) - Coerces []string or []any of strings to []string (returns a copy)
  • Comprehensive tests: Added internal/rules/settings/settings_test.go with table-driven tests covering all type branches (int, float64, int64, string, bool, nil, slices)

  • Refactored 10 rule packages to use the shared helpers:

    • tablereadability, tableformat, tokenbudget, linelength, paragraphreadability, paragraphstructure, concisenessscoring, nomultipleblanks, maxfilelength, firstlineheading
  • Removed duplicate code: Deleted private toInt() and toFloat() implementations from each rule package

Implementation Details

  • All helper functions reject unsupported types (strings, bools, nil, slices) consistently across all rules
  • ToStringSlice returns a fresh copy to prevent caller mutations from affecting the original input
  • Float-to-int conversion truncates toward zero (e.g., 3.9 → 3, -2.7 → -2)
  • Parameter renamed from settings to s in ApplySettings methods to avoid shadowing the package name

https://claude.ai/code/session_01EJJ9LCv67GJ39SnJNpYGx3

Copilot AI review requested due to automatic review settings April 20, 2026 16:10
@codecov

codecov Bot commented Apr 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.06%. Comparing base (3291efc) to head (3778fb9).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   86.76%   87.06%   +0.30%     
==========================================
  Files          96       97       +1     
  Lines       10446    10346     -100     
==========================================
- Hits         9063     9008      -55     
+ Misses        900      856      -44     
+ Partials      483      482       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates repeated “untyped settings map → typed Go value” coercion logic across multiple Markdown lint rules into a shared internal/rules/settings package, and updates affected rules/tests accordingly to reduce duplication and improve consistency.

Changes:

  • Added internal/rules/settings with exported coercion helpers (ToInt, ToFloat, ToStringSlice) plus table-driven tests.
  • Refactored several rules to use the shared helpers and renamed ApplySettings(settings ...) params to avoid package-name shadowing.
  • Removed now-redundant per-rule toInt/toFloat helpers and their coverage-only tests; updated plan status tracking.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/rules/settings/settings.go Introduces shared coercion helpers used by multiple rule packages.
internal/rules/settings/settings_test.go Adds table-driven unit tests for the shared coercion helpers.
internal/rules/tokenbudget/rule.go Replaces local numeric coercion with settings.ToInt/ToFloat; avoids settings name shadowing.
internal/rules/tablereadability/rule.go Switches numeric coercion to shared helpers; avoids settings name shadowing.
internal/rules/tablereadability/rule_coverage_test.go Removes coverage-only tests for deleted local coercion helpers.
internal/rules/tableformat/rule.go Uses shared settings.ToInt and removes local toInt.
internal/rules/tableformat/rule_coverage_test.go Removes coverage-only tests for deleted local toInt.
internal/rules/paragraphstructure/rule.go Switches to shared settings.ToInt; avoids settings name shadowing.
internal/rules/paragraphreadability/rule.go Switches to shared settings.ToInt/ToFloat; avoids settings name shadowing.
internal/rules/nomultipleblanks/rule.go Switches to shared settings.ToInt; removes local toInt.
internal/rules/maxfilelength/rule.go Switches to shared settings.ToInt; removes local toInt.
internal/rules/linelength/rule.go Switches numeric coercion to shared settings.ToInt; leaves local toStringSlice.
internal/rules/firstlineheading/rule.go Switches to shared settings.ToInt; removes local toInt.
internal/rules/concisenessscoring/rule.go Switches to shared settings.ToInt/ToFloat; removes local helpers.
plan/85_coverage-to-95-percent.md Updates plan status and checks off completed Phase 1 tasks.
PLAN.md Updates the generated plan catalog row to reflect the new plan status.

Comment thread internal/rules/settings/settings.go
Comment thread internal/rules/linelength/rule.go Outdated
@jeduden jeduden changed the title Extract type conversion helpers into shared settings package Plan 85: extract shared type-conversion helpers (Phase 1) Apr 20, 2026
Phase 1 of plan 85: Replace 10 duplicated private toInt copies and 4
toFloat copies with a single internal/rules/settings package covered
by table-driven unit tests.

Each rule package's ApplySettings parameter is renamed from settings
to s so the package can be imported under its natural name. The new
settings package has 100% statement coverage. The emptysectionbody
and maxsectionlength variants are kept local because they reject
non-whole floats.
@jeduden
jeduden force-pushed the claude/plan-85-shared-helpers branch from 22b4b7f to 8cee9dd Compare April 20, 2026 18:47
@jeduden
jeduden requested a review from Copilot April 20, 2026 18:47
- ToInt/ToFloat reject NaN, +/-Inf, and out-of-range floats so
  invalid config surfaces deterministically instead of yielding
  implementation-dependent int values.
- linelength.applyExclude now uses settings.ToStringSlice so rule
  state does not alias the caller's config slice; drops the local
  toStringSlice duplicate.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Comment thread internal/rules/settings/settings.go
Comment thread internal/rules/settings/settings.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Apr 21, 2026
@jeduden

jeduden commented Apr 21, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented Apr 21, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-150-1776751956. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden

jeduden commented Apr 21, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit 24656d9. CI run that validated the merge.

Next: Done — nothing more to do here.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Apr 21, 2026
@jeduden
jeduden merged commit 24656d9 into main Apr 21, 2026
16 checks passed
jeduden pushed a commit that referenced this pull request Apr 21, 2026
PR #150 landed the settings helpers during our PR's review window.
Swap MDS037's local toInt and toStringSlice for the package-level
versions so the rule picks up the project-wide coercion semantics:
settings.ToInt truncates fractional floats (instead of rejecting
them) and rejects NaN/Inf/out-of-range. Update the fractional-float
test accordingly.
jeduden pushed a commit that referenced this pull request Apr 21, 2026
PR #150 landed the settings helpers during our PR's review window.
Swap MDS037's local toInt and toStringSlice for the package-level
versions so the rule picks up the project-wide coercion semantics:
settings.ToInt truncates fractional floats (instead of rejecting
them) and rejects NaN/Inf/out-of-range. Update the fractional-float
test accordingly.
jeduden pushed a commit that referenced this pull request Apr 22, 2026
PR #150 landed the settings helpers during our PR's review window.
Swap MDS037's local toInt and toStringSlice for the package-level
versions so the rule picks up the project-wide coercion semantics:
settings.ToInt truncates fractional floats (instead of rejecting
them) and rejects NaN/Inf/out-of-range. Update the fractional-float
test accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants